fix(api): bound and truncate api signatures#6820
Conversation
b13b51a to
dfe3346
Compare
dfe3346 to
95599ad
Compare
95599ad to
a9bdbae
Compare
ce3b0d9 to
c859797
Compare
c859797 to
8dc1d8d
Compare
…provedlist-sig-limit
halibobo1205
left a comment
There was a problem hiding this comment.
LGTM.
[SHOULD] Please make sure this behavior change is called out in the release notes / API changelog, so SDK and client owners can adapt.
There was a problem hiding this comment.
Key design concern: getTransactionApprovedList semantic breaking change (all-or-nothing vs partial list). Author acknowledged this is intentional and will add release notes — please ensure that lands before merge.
An error will occur if a signature in the transmitted signature list is not authorized.
| return ECDSASignature.fromComponents(rsv.getR(), rsv.getS(), rsv.getV()).toBase64(); | ||
| } | ||
|
|
||
| public static Transaction truncateSignatures(Transaction trx) { |
There was a problem hiding this comment.
[NIT] truncateSignatures truncates oversized signatures to 65 bytes — this is a tolerance policy that only the read-only query APIs need (because they echo back the transaction). The chainbase capsule layer should not carry caller-specific API concerns; it is a data-encapsulation layer shared by consensus and storage paths that do not need this behavior.
Suggestion: move truncateSignatures out of TransactionCapsule and into the call sites (Wallet or a framework-layer helper). The method is currently only called by two API-layer methods, so the move is low-risk.
| assertEquals(GrpcAPI.TransactionApprovedList.Result.response_code.OTHER_ERROR, | ||
| rejected.getResult().getCode()); | ||
| assertEquals(0, rejected.getApprovedListCount()); | ||
| Assert.assertTrue(rejected.getResult().getMessage().contains("more than key counts")); |
There was a problem hiding this comment.
[NIT] Two small follow-ups on the new tests and truncateSignatures:
-
WalletTest.javaline 1489 — assertion depends on internal exception message format.Assert.assertTrue(rejected.getResult().getMessage().contains("more than key counts"))relies on the exact text produced bycheckWeight'sPermissionExceptionpropagated throughcatch (Exception ex)message formatting. If the exception message or the catch formatting changes, the test becomes a false pass. Consider asserting only onresponse_code == OTHER_ERRORand that the message is non-empty. -
TransactionCapsule.javaline 469-488 —truncateSignaturesiterates the signature list twice. First pass setsneedTruncate, second pass rebuilds. SincegetTotalSignNumcaps the list at 5 in practice the overhead is negligible, but a single-pass version (alwaysclearSignature()+ re-add each sig, truncating inline) would be simpler.
Suggestion: tighten the test assertion in item 1; optionally simplify the two-pass loop in item 2.
What does this PR do?
Hardens the two read-only signature APIs —
getTransactionApprovedList(/wallet/getapprovedlist) andgetTransactionSignWeight— against CPU exhaustion and oversized-signature echo-back.1. Bound the recovery loop (CPU DoS)
Wallet.getTransactionApprovedListpreviously ran a hand-rolled unboundedecrecoverloop. Replaced by delegation toTransactionCapsule.checkWeight, matchinggetTransactionSignWeight.checkWeightrejects up front whensigs.size() > permission.getKeysCount(), before any signature recovery.2. Reject excessive signature counts at entry
Both APIs now check
trx.getSignatureCount() > getTotalSignNum()as the very first step, returningOTHER_ERROR/ "too many signatures" immediately. This is a zero-cost guard that mirrors the bound already enforced by the consensus signature-verification path.3. Truncate oversized signatures at entry
TransactionCapsule.truncateSignatures(trx)truncates any signature > 65 bytes to its first 65 bytes. The truncated bytes are copied out (ByteString.copyFrom(sig.substring(...).toByteArray())) rather than kept as aByteString.substringview — a bounded view would still pin the original oversized backing array, so the copy lets it be released. Called at the top of both methods; the echoed-back transaction is set on the response builder at entry, so no path leaks untruncated signatures.4. Harden checkWeight error message
weight == 0path previously hex-encodedsig.toByteArray()(potentially oversized) into the exception message. Changed to the transaction hash (hash), a fixed 32-byte value that is more useful for debugging and cannot bloat error responses. This protects all callers ofcheckWeight, including consensus paths.5. Keep the HTTP error path intact for result-only responses
The "too many signatures" early-return builds a result-only response with no
transactionfield.Util.printTransactionSignWeight/printTransactionApprovedListpreviously assumedtransactionalways exists, so on the HTTP servlet path they threw an NPE and the endpoint returned a generic servlet error instead of the intendedOTHER_ERROR/ "too many signatures". Both helpers now skip the nested-transaction rewrite when no transaction is present.Why are these changes required?
getTransactionApprovedListwas the only signature-recovery path without a bound on signature count. A single unauthenticated request with thousands of padded signatures could trigger unboundedecrecovercalls. Signature truncation, the memory-safe copy, and the error-message fix ensure clean, bounded, canonical responses on every code path — including the HTTP endpoints.This PR has been tested by
testApprovedListSigBound— signature withinkeysCountsucceeds; exceedingkeysCountrejected before recoverytestApprovedListSigTruncate— 70-byte signature truncated to 65 bytes, signer still recoveredtestApprovedListTooManySigs— exceedinggetTotalSignNum()rejected at entry with "too many signatures"testSignWeightSigTruncate— same truncation test forgetTransactionSignWeighttestSignWeightTooManySigs— same total-sign-num guard test forgetTransactionSignWeighttestPrintApprovedListTooManySigsHttpPath/testPrintSignWeightTooManySigsHttpPath— HTTP print helpers return theOTHER_ERROR/ "too many signatures" JSON (notransactionfield) instead of throwingWalletTest,TransactionUtilTest,UtilTest,RpcApiServicesTest— all green:framework:checkstyleMain,:framework:checkstyleTest— cleanFollow up
None.
Extra details
getTransactionApprovedListnow delegates tocheckWeight, applying the same permission semantics asgetTransactionSignWeight. Behavioral changes:permission.getKeysCount().Callers that relied on the old "recover any signature" behavior to probe arbitrary signer addresses will see stricter results. No database, consensus, or config changes; read-only API paths only.